[client-v2, jdbc-v2] Support for Time and Time64 #2487
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (8)
- clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseDataType.java (243-244) The Time and Time64 types are defined with LocalDateTime.class as their object type, but the mapping in dataTypeClassMap() doesn't include LocalDateTime. For consistency with other date/time types like DateTime and DateTime64, consider adding LocalDateTime and possibly ZonedDateTime to the mapping.
- clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseDataType.java (243-244) There's an inconsistency in the supported types between Time and Time64. Time64 supports BigInteger but Time doesn't. If Time64 is a higher precision version of Time, consider either adding BigInteger support to Time as well or documenting why only Time64 supports BigInteger.
- clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseDataType.java (131-132) The Time and Time64 types are defined with LocalDateTime.class as their object type. If these types represent only time values without date components, consider using LocalTime.class instead. If they do include date components, a comment explaining this would be helpful.
- client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java (183-183) The implementation for Time64 returns a BigInteger while Time returns a Long. This inconsistency might cause issues for callers expecting the same type. Consider either documenting this difference or making the return types consistent.
- client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/MapBackedRecord.java (133-134) The implementation for Time type assumes the value is stored as a long that can be directly converted to epoch seconds. Consider checking the actual type of the stored value before conversion, as other time-related types in this method use direct casting rather than going through getLong().
-
client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java (654-656)
The test is using a hardcoded string value for the Time64 test assertion. Consider calculating this value dynamically to make the test more robust against implementation changes:
new String[]{ String.valueOf(DataTypeUtils.toNanoseconds(maxTime64)) }); - client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java (659-662) The implementation for serializing Instant to Time64 combines seconds and nanos by shifting seconds left by 32 bits and adding nanos. This approach might not match ClickHouse's Time64 internal representation, which typically stores time as nanoseconds since midnight. Consider verifying this implementation against ClickHouse's expected format.
- client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java (554-556) The Time data type case only converts values to Integer. Consider adding support for more time-related types like LocalTime or java.sql.Time, which would make the API more flexible.
💡 To request another review, post a new comment with "/windsurf-review".
Comment on lines
454
to
456
| case Time: | ||
| case Time64: | ||
| return readValue(colName); |
There was a problem hiding this comment.
For Time/Time64 types, the code assumes readValue() returns an Instant object. Is this guaranteed by the implementation? If not, there might be a ClassCastException when handling these types.
There was a problem hiding this comment.
This is a good point! Will verify.
client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java
Show resolved
Hide resolved
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java
Fixed
Show resolved
Hide resolved
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java
Fixed
Show fixed
Hide fixed
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java
Fixed
Show fixed
Hide fixed
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java
Fixed
Show fixed
Hide fixed
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java
Fixed
Show resolved
Hide resolved
mzitnik
approved these changes
Jul 8, 2025
|
Thank you @mzitnik ! I need to add more tests for JDBC part then will test with UI. |
4 tasks
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2460
Checklist
Delete items not relevant to your PR: